Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ownership for MiqRequest in RBAC #17208

Merged
merged 6 commits into from
Mar 28, 2018
Merged

Add ownership for MiqRequest in RBAC #17208

merged 6 commits into from
Mar 28, 2018

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Mar 27, 2018 •
edited
Loading

Self service and limited self service users will see only his or his group's users miq_requests after this fix.

For ownership of MiqRequests to users we are using requester_id.

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1545395

@miq-bot assign @gtanzillo

@miq-bot miq-bot added the wip label Mar 27, 2018
@lpichler lpichler force-pushed the restrict_miq_request_by_users_group branch from 2f8885a to 8918547 Compare March 27, 2018 15:55
@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2018

Checked commit lpichler@8918547 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@lpichler
Rename method
e163934 
from get_self_service_objects to
self_service_ownership_scope
@lpichler
Use conjunction(De-Morgan rules)
4a50b5a 
¬(A∨B∨C)=¬A∧¬B∧¬C

return nil if miq_group.nil? || !miq_group.self_service? || !(klass < OwnershipMixin)

to

return unless !(miq_group.nil? || !miq_group.self_service? || !(klass < OwnershipMixin))

to

return unless !miq_group.nil? && !!miq_group.self_service? && !!(klass < OwnershipMixin)

to

return unless miq_group.present? && miq_group.self_service? && klass < OwnershipMixin

to(add to method)

return nil unless self_service_ownership_scope?(miq_group, klass)
@lpichler
User array to use ownership feature by more target models
80f4f10
@lpichler
Require scope user_or_group_owned for ownership target classes
bb98fea
@lpichler
Support ownership scope for MiqRequest model
97324fc
@lpichler lpichler force-pushed the restrict_miq_request_by_users_group branch from 8918547 to 97324fc Compare March 28, 2018 12:28
@lpichler lpichler changed the title [WIP] Restrict MiqRequest by user's group Add ownership for MiqRequest in RBAC Mar 28, 2018
@gtanzillo gtanzillo self-assigned this Mar 28, 2018
@gtanzillo gtanzillo merged commit 0969759 into ManageIQ:master Mar 28, 2018
@lpichler lpichler deleted the restrict_miq_request_by_users_group branch March 28, 2018 13:58
@simaishi
Copy link
Contributor

simaishi commented Apr 2, 2018

@lpichler @gtanzillo Can this be fine/yes?

simaishi pushed a commit that referenced this pull request Apr 2, 2018
@gtanzillo @simaishi
Merge pull request #17208 from lpichler/restrict_miq_request_by_users…
a1ec518 
…_group

Add ownership for MiqRequest in RBAC
(cherry picked from commit 0969759)

https://bugzilla.redhat.com/show_bug.cgi?id=1562777
@simaishi
Copy link
Contributor

simaishi commented Apr 2, 2018

Gaprindashvili backport details:

$ git log -1
commit a1ec51830e537bb4d29099051675d7ab006d1d46
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Wed Mar 28 09:28:38 2018 -0400

    Merge pull request #17208 from lpichler/restrict_miq_request_by_users_group
    
    Add ownership for MiqRequest in RBAC
    (cherry picked from commit 09697591a48eb2d1d994ec57d09a842844c0cab7)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1562777

@gtanzillo
Copy link
Member

@simaishi I'm ok with fine/yes but, since @lpichler did the work, I want to make sure he's comfortable with it befioe I set the label.

@lpichler
Copy link
Contributor Author

lpichler commented Apr 3, 2018

@simaishi No, there are conflicts, I will create a FINE PR.

lpichler pushed a commit to lpichler/manageiq that referenced this pull request Apr 3, 2018
@gtanzillo @lpichler
Merge pull request ManageIQ#17208 from lpichler/restrict_miq_request_…
832be6c 
…by_users_group

Add ownership for MiqRequest in RBAC
@lpichler lpichler mentioned this pull request Apr 3, 2018
Merged
@simaishi
Copy link
Contributor

Backported to Fine via #17245

yrudman added a commit to yrudman/manageiq that referenced this pull request May 23, 2018
@yrudman
MiqRequest was added to RBAC in ManageIQ#17208 but did not include Ac…
582d4ab 
…tAsTaggable concern.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576129
@yrudman yrudman mentioned this pull request May 23, 2018
Merged
@agrare agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Jun 4, 2018
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@gtanzillo @d-m-u
Merge pull request ManageIQ#17208 from lpichler/restrict_miq_request_…
fa4670a 
…by_users_group

Add ownership for MiqRequest in RBAC
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 29, 2018
@jrafanie
Remove Request taggable and prevent tag filtering
7aa3716 
MiqRequest was changed to allow ownership for self service and limited
self-service users in ManageIQ ManageIQ#17208, BZ: 1545395

This caused a problem if you had tag filters assign to a user's group
undefined method `find_tags_by_grouping'.  This was fixed in
ManageIQ ManageIQ#17466, BZ: 1576129, and shipped with:

Fine: https://bugzilla.redhat.com/show_bug.cgi?id=1583711
Gaprindindashvili: https://bugzilla.redhat.com/show_bug.cgi?id=1583710

Unfortunately, this second fix to add taggable caused a new bug: users in
groups having tag filters could not see their own requests.

This commit changes MiqRequest to no longer be taggable, since it's not
even taggable in the UI and instead, we add MiqRequest to a list of
models that are RBAC'able but not taggable so we don't try to filter
MiqRequest based on a user's group tag filters.

Credit goes to github user LorkScorguar who reported this issue and
provided lots of diagnostics to help us fix this properly.

For gaprindashvili and fine:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596738

For hammer:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576129
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 29, 2018
@jrafanie
Remove Request taggable and prevent tag filtering
bf8fb93 
MiqRequest was changed to allow ownership for self service and limited
self-service users in ManageIQ ManageIQ#17208, BZ #1545395

This caused a problem if you had tag filters assign to a user's group
undefined method `find_tags_by_grouping'.  This was fixed in
ManageIQ ManageIQ#17466, BZ #1576129, and shipped with:

Fine: BZ #1583711
Gaprindindashvili: BZ #1583710

Unfortunately, this second fix to add taggable caused a new bug: users in
groups having tag filters could not see their own requests.

This commit changes MiqRequest to no longer be taggable, since it's not
even taggable in the UI and instead, we add MiqRequest to a list of
models that are RBAC'able but not taggable so we don't try to filter
MiqRequest based on a user's group tag filters.

Credit goes to github user LorkScorguar who reported this issue and
provided lots of diagnostics to help us fix this properly.

For gaprindashvili and fine:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596738

For hammer:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576129
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 29, 2018
@jrafanie
Remove Request taggable and prevent tag filtering
36a25f2 
MiqRequest was changed to allow ownership for self service and limited
self-service users in ManageIQ ManageIQ#17208, BZ #1545395

This caused a problem if you had tag filters assign to a user's group
undefined method `find_tags_by_grouping'.  This was fixed in
ManageIQ ManageIQ#17466, BZ #1576129, and shipped with:

Fine: BZ #1583711
Gaprindindashvili: BZ #1583710

Unfortunately, this second fix to add taggable caused a new bug: users in
groups having tag filters could not see their own requests.

This commit changes MiqRequest to no longer be taggable, since it's not
even taggable in the UI and instead, we add MiqRequest to a list of
models that are RBAC'able but not taggable so we don't try to filter
MiqRequest based on a user's group tag filters.

Credit goes to github user LorkScorguar who reported this issue and
provided lots of diagnostics to help us fix this properly.

To test this, simply assign managed filters to a user's group, such as
/managed/environments/production, create a request for that user and
try to see that user's request.  They couldn't see it if they received
the intermediate fix, ManageIQ#17466, or if they didn't receive that fix, they'd
receive the `find_tags_by_grouping` error shown above.

For gaprindashvili and fine:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596738

For hammer:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576129
@jrafanie jrafanie mentioned this pull request Jun 29, 2018
Merged
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 29, 2018
@jrafanie
Remove Request taggable and prevent tag filtering
f8c0553 
MiqRequest was changed to allow ownership for self service and limited
self-service users in ManageIQ ManageIQ#17208, BZ #1545395

This caused a problem if you had tag filters assign to a user's group
undefined method `find_tags_by_grouping'.  This was fixed in
ManageIQ ManageIQ#17466, BZ #1576129, and shipped with:

Fine: BZ #1583711
Gaprindindashvili: BZ #1583710

Unfortunately, this second fix to add taggable caused a new bug: users in
groups having tag filters could not see their own requests.

This commit changes MiqRequest to no longer be taggable, since it's not
even taggable in the UI and instead, we add MiqRequest to a list of
models that are RBAC'able but not taggable so we don't try to filter
MiqRequest based on a user's group tag filters.

Credit goes to github user LorkScorguar who reported this issue and
provided lots of diagnostics to help us fix this properly.

To test this, simply assign managed filters to a user's group, such as
/managed/environments/production, create a request for that user and
try to see that user's request.  They couldn't see it if they received
the intermediate fix, ManageIQ#17466, or if they didn't receive that fix, they'd
receive the `find_tags_by_grouping` error shown above.

For gaprindashvili and fine:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596738

For hammer:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576129
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees

@gtanzillo gtanzillo

Labels
blocker bug fine/backported gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lpichler @miq-bot @simaishi @gtanzillo @agrare